-
Notifications
You must be signed in to change notification settings - Fork 0
feat: OCP maps/pen 42 #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…raform-aws-vpc into ocp-maps/pen-42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for all of the remarks, but a lot of them are the same thing, just in different locations.
Let's have a conversation about them 😄
main.tf
Outdated
@@ -115,7 +116,10 @@ resource "aws_subnet" "public" { | |||
{ | |||
Name = try( | |||
var.public_subnet_names[count.index], | |||
format("${var.name}-${var.public_subnet_suffix}-%s", element(var.azs, count.index)) | |||
format("${var.name_prefix}%s-%s-sub-${var.public_subnet_suffix}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over here we have no dash (-
) between the name_prefix
and the next string, but we do have a dash before the suffix.
Is this the expected approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes:
- Add the after
${var.name_prefix}
- Add
${var.region_shortcode}
after name_prefix
main.tf
Outdated
@@ -115,7 +116,10 @@ resource "aws_subnet" "public" { | |||
{ | |||
Name = try( | |||
var.public_subnet_names[count.index], | |||
format("${var.name}-${var.public_subnet_suffix}-%s", element(var.azs, count.index)) | |||
format("${var.name_prefix}%s-%s-sub-${var.public_subnet_suffix}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes:
- Add the after
${var.name_prefix}
- Add
${var.region_shortcode}
after name_prefix
tgw.tf
Outdated
) | ||
} | ||
|
||
# There are as many routing tables as the number of NAT gateways |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This will add TGW subnets and creation of an attachment to the VPC module. Also naming is changed for the resources to reflect the naming recommendations.